Skip to content

[fcos] templates: place /etc/pivot/kernel-args on masters/workers#1236

Merged
openshift-merge-robot merged 1 commit intoopenshift:fcosfrom
vrutkovs:fcos-kernel-tweaks
Nov 1, 2019
Merged

[fcos] templates: place /etc/pivot/kernel-args on masters/workers#1236
openshift-merge-robot merged 1 commit intoopenshift:fcosfrom
vrutkovs:fcos-kernel-tweaks

Conversation

@vrutkovs
Copy link
Contributor

@vrutkovs vrutkovs commented Nov 1, 2019

This would disable cgroupsv2 and Spectre mitigations. These files are served by MCD to ensure correct initial hash on masters is being calculated

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 1, 2019
@vrutkovs vrutkovs changed the title templates: place /etc/pivot/kernel-args on masters/workers [fcos] templates: place /etc/pivot/kernel-args on masters/workers Nov 1, 2019
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019

/hold

Testing it locally

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2019
templates: place /etc/pivot/kernel-args on masters/workers
ea203ec 
This would disable cgroupsv2 and Spectre mitigations
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019

/hold cancel

Works fine here

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2019
@cgwalters
Copy link
Member

cgwalters commented Nov 1, 2019

This should use the encapsulated MachineConfig with kernelArguments.

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019

This should use the encapsulated MachineConfig with kernelArguments.

I was trying to add a new MC via installer, but that made node annotations to have a wrong rendered machine config hash. Eventually all this would be cleaned up

@ericavonb
Copy link

ericavonb commented Nov 1, 2019 •
edited
Loading

I was trying to add a new MC via installer, but that made node annotations to have a wrong rendered machine config hash.

That shouldn't happen. How were you adding the new machineconfig?

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019

/lgtm
/override ci/prow/e2e-aws

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 1, 2019

@vrutkovs: you cannot LGTM your own PR.

Details

In response to this:

/lgtm
/override ci/prow/e2e-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 1, 2019

@vrutkovs: Overrode contexts on behalf of vrutkovs: ci/prow/e2e-aws

Details

In response to this:

/lgtm
/override ci/prow/e2e-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019 •
edited
Loading

That shouldn't happen. How were you adding the new machineconfig?

Via the installer - openshift/installer@1f837e7

https://kubernetes.slack.com/archives/C6BRQSH2S/p1572621344249600:

riiiiight, so during bootstrap node annotations has a hash of expected machineconfig and later on MCD won't reboot this node as its expected to have the config already

but the trick is that installer creates a new MC with fcos tweaks (in RHCOS it would disable hyperthreading) - and thus rendered machineconfig has a different hash

and MCP can't proceed cause initial rendered config has never in fact existed

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019

/lgtm

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 1, 2019

@vrutkovs: you cannot LGTM your own PR.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@LorbusChris
Copy link
Contributor

LorbusChris commented Nov 1, 2019

ok, let's do this for now. We can do any clean ups/polishing of this a bit further down the road.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 1, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LorbusChris, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [LorbusChris,vrutkovs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019

I agree this is certainly a bad hack and would be reverted, but without e2e its hard to come up with a better fix

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 1, 2019

@vrutkovs: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws ea203ec link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Nov 1, 2019

/override ci/prow/e2e-aws

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 1, 2019

@vrutkovs: Overrode contexts on behalf of vrutkovs: ci/prow/e2e-aws

Details

In response to this:

/override ci/prow/e2e-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit 56c9118 into openshift:fcos Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kikisdeliveryservice kikisdeliveryservice Awaiting requested review from kikisdeliveryservice

@LorbusChris LorbusChris Awaiting requested review from LorbusChris

Assignees

@LorbusChris LorbusChris

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vrutkovs @cgwalters @ericavonb @openshift-ci-robot @LorbusChris @openshift-merge-robot